This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

Follow-up to my earlier review comment. That pass reviewed the diff file-by-file; this pass audited the PR along four cross-cutting axes instead: (1) workspace provenance at every store call site, (2) workspace provenance at every client request-construction site, (3) the full cross-object reference graph under delete/recreate, and (4) a cross-layer behavior matrix (server/CLI/TUI/SDK/three drivers). Same scope as before: fresh install assumed, upgrade paths out of scope, missing authz deferred to Phase 2. The new findings are mostly relational — they live between components rather than inside any one file, which is why the first pass missed them.

Additional must-fix candidates

1. Watch-adoption path persists driver-supplied workspaces verbatim — one ghost record halts gateway-wide reconciliation

The create branch of apply_sandbox_update_locked (crates/openshell-server/src/compute/mod.rs:1163-1196) clones incoming.workspace from the driver event and writes via raw put_if, bypassing the requires_workspace() guard that every typed write path enforces (persistence/mod.rs:431, 618, 744). Three feeders can deliver an empty workspace on a fresh install:

• The VM driver drops workspace entirely when building watch/status snapshots (crates/openshell-driver-vm/src/driver.rs:4980-4994, ..Default::default()), so any delete/watch race re-adopts a sandbox with workspace: "".
• Podman forwards "" for a container carrying openshell.managed=true + a sandbox-id label but no workspace label (driver-podman/src/watcher.rs:320-326) — reachable by manually labeling a container.
• Docker does the same via unwrap_or_default() (driver-docker/src/lib.rs:2781-2784).

Consequences of the resulting ""-workspace record: it is unreachable by every workspace-scoped RPC ("" normalizes to "default" on lookup, so get/delete return NOT_FOUND), undeletable via the API, fails the requires_workspace guard on every subsequent update_message_cas — and because reconcile_store_with_backend propagates that error with ? before its prune loop (compute/mod.rs:1057-1086), one ghost record stops orphan pruning for the entire gateway. It surfaces only as a blank WORKSPACE cell under --all-workspaces. This also contradicts RFC 0011's stated fallback ("the sandbox is assigned to the default workspace").

Fix shape: normalize/validate the workspace at the adoption boundary (empty → "default", or refuse adoption) and fix the VM driver to round-trip workspace in snapshots.

2. Inference routes reference providers by name with no delete guard and no cleanup — silent traffic rebinding

delete_provider_record (crates/openshell-server/src/grpc/provider.rs:312-345) checks only sandbox attachments; inference routes neither block provider deletion nor get cleaned up, and resolve_route_by_name (inference.rs:984-996) re-resolves the provider by name on every bundle fetch. Sequence: inference set --provider foo → delete provider foo (succeeds) → recreate foo pointing at a different backend/credentials → every sandbox in the workspace transparently starts sending inference traffic through (and receiving the api_key of) the new provider, with no route update and no audit event.

Secondary defect on the same edge: while the route dangles, the ? in resolve_inference_bundle (inference.rs:913-917) turns one broken route into a failure of the entire bundle — a dangling route also takes down delivery of the other routes for the workspace.

The codebase's own invariant elsewhere (the sandbox-attachment guard) is that name references must not dangle, so this is a gap, not name-as-identity by design. Fix shape: include inference routes in the provider delete guard (or cascade-delete them), and make resolve_inference_bundle skip-and-log a dangling route instead of failing the bundle.

3. Provider deletion doesn't synchronize with attach — dangling attachment adopts a recreated provider's credentials

handle_delete_provider (provider.rs:2484-2493) never acquires sandbox_sync_guard, while handle_attach_sandbox_provider validates provider existence before taking the guard (grpc/sandbox.rs:356-369). An attach racing a delete leaves a by-name attachment in spec.providers pointing at nothing; recreating the provider under the same name silently injects the new provider's credentials into that sandbox. Same TOCTOU shape as the workspace-member race noted in the earlier comment, but on the edge that carries credentials. Fix shape: take sandbox_sync_guard in the delete path (or re-validate provider existence inside the guarded section of attach).

4. Service endpoints are never cleaned up on sandbox delete

cleanup_sandbox_owned_records (compute/mod.rs:1384-1405) removes SSH sessions, settings, policy revisions, and draft chunks — service_endpoint is absent from the list, and no other sandbox-delete path touches it. Orphaned endpoints then (a) permanently block workspace deletion via the "service" blocker in the DeleteWorkspace guard, with no way to remove them except DeleteService against a sandbox that no longer exists, and (b) after recreating a same-name sandbox, Get/ListServices present the stale endpoint (old target_port) as belonging to the new sandbox while routing 404s (the stored sandbox UUID fails closed — the one place the dual name+UUID pattern pays off). Fix shape: add service endpoints to the cleanup list.

Worth fixing, borderline blocking

• TUI ignores --workspace / OPENSHELL_WORKSPACE entirely: openshell term --workspace team-ml parses successfully and is dropped — main.rs:3471 never passes cli.workspace and the TUI hardcodes "default" (openshell-tui/src/app.rs:891).
• TUI provider create has the same wrong-workspace targeting as sandbox create (openshell-tui/src/lib.rs:1616): a provider — including its credentials — is silently created in the last-cycled workspace while the header shows "all". If the fix for the sandbox-create finding is generalized to cover both create paths, this comes for free.
• Python SDK Sandbox context manager can rebind a SandboxRef across workspaces (python/openshell/sandbox.py:810-836): attaching with a ref from workspace A while the constructor says workspace="default" silently rebinds the session to a same-named default-workspace sandbox; exec/delete then target the wrong sandbox. Prefer ref.workspace when set, or reject a mismatch.
• Sandbox settings can rebind across delete/recreate (race): settings are keyed by (workspace, name) with no sandbox back-pointer, and UpdateConfig (settings_mutex) and sandbox deletion (compute sync_lock) share no lock. A settings write landing after cleanup leaves a record that silently applies to the next same-named sandbox — since settings carry the agentic approval mode, a stale "auto-approve" attaching to a fresh sandbox is a policy-bypass shape.
• Workspace-not-found and resource-not-found are indistinguishable by code, and two clients act on the wrong meaning: SDK wait_deleted("box", workspace="typo") treats any NOT_FOUND as successful deletion (sandbox.py:527-542); the CLI service-forward loop reports "sandbox no longer exists" on a workspace typo (run.rs:2983-2986).
• OCSF audit events drop workspace even where it is already in hand: service_routing.rs:597 parses the workspace out of the Host header and discards it (_workspace) before emitting the HTTP-rejection event, and the endpoint ConfigStateChange events omit it despite endpoint.metadata.workspace being on the struct — denied requests against same-named sandboxes in different workspaces produce identical audit rows. Full attribution is Phase 5, but these sites already hold the value.
• Podman discovery is not gateway-scoped: it filters on openshell.managed=true alone (driver-podman/src/container.rs:44-46) where Docker also filters on namespace. Two gateways sharing a Podman host ingest each other's containers — which compounds with item 1, since adoption skips workspace validation.

Non-blocking notes

Docs/help claim workspace names are DNS-1123/63 chars while the server enforces 19 (proto/openshell.proto:2023, main.rs:2013 vs grpc/workspace.rs:51-58); the SSH alias format implemented (openshell-{name}.{workspace}) differs from the format documented in RFC 0011; sandbox get detail output and provider JSON omit the workspace; cleanup_sandbox_ssh_sessions reads a single page of 1000 with no pagination loop (compute/mod.rs:1407-1430), unlike its sibling scans; race-orphaned UUID-scoped records (policy revisions, draft chunks, refresh state) are unreachable by any API yet block workspace deletion forever via the guard's safety net; the SDK accepts workspace="" without validation.

Phase 2 readiness (informational — no action needed in this PR)

Classified all 73 RPCs plus the data-plane paths for "can a membership check bolt on here." Good news: the resolve_workspace choke point makes ~40 request-scoped endpoints genuine bolt-ons (one mechanical change: thread the principal into it), and all nine TODO(phase2) markers map to real gaps — none stale. Three surfaces will need rework rather than a bolt-on: GetSandboxLogs tails by client-supplied sandbox_id with no resource fetch (already marked); the browser-facing HTTP service-routing proxy runs before auth with no principal at all (unmarked — worth a marker now); and the platform-scope operations (profile "" scope, global:true config/policy, GetGatewayConfig) have no carrier for a platform-admin role check. Bolt-on-ready but unmarked: ExecSandboxInteractive, RevokeSshSession, the bearer-user path of GetSandboxConfig, Set/GetInferenceRoute, and membership filtering on ListWorkspaces/GetWorkspace. Streams verify at establish time only, and SSH revocation does not sever live ForwardTcp tunnels.

What the audit confirmed clean

For completeness, the coverage side: all 118 production store call sites classified (72 via resolve_workspace/resolve_profile_workspace, 24 via trusted ID-based lookups, 13 justified platform-scope "", zero hardcoded "default" — the adoption path in item 1 was the single unjustified site); all 112 client request-construction sites across CLI/TUI/SDK/sandbox-runtime classified with no unjustified empty-workspace sends beyond the findings above; the full cross-object reference graph over all 14 record types tabled — the UUID-revalidated paths (service proxy, SSH forward) are fully delete/recreate-safe, and the DNS label math, hostname parse round-trip, and driver name-length backstops (docker 200, podman 255, k8s 63) all hold under the 19-char caps.

Feedback from my agent review

[P1] Do not discard startup activity before workspace discovery

Locations:

• crates/openshell-sandbox/src/lib.rs:364
• crates/openshell-sandbox/src/lib.rs:409

The denial and activity aggregators drain a batch before invoking their flush callbacks. When the workspace is not yet known, these callbacks return successfully, so the drained events are permanently lost. This can drop policy proposals and activity telemetry during normal sandbox startup.

Buffer the batch until workspace discovery completes, wait on the workspace receiver before starting the aggregators, or return a retryable error so the aggregator retains the batch.

[P1] Preserve global profile scope throughout provider creation

Location: crates/openshell-cli/src/run.rs:4910

provider create --global-profile initially validates a custom profile using profile_workspace, but the --from-existing and --runtime-credentials paths subsequently fetch the profile using the provider's workspace. A valid global custom profile therefore fails unless a same-named workspace profile exists. If one does exist, discovery and credential validation can use that different profile while the persisted provider remains bound to the global profile.

Pass profile_workspace through every profile lookup involved in discovery and runtime-credential validation.

[P1] Prevent resource creation from racing workspace deletion

Location: crates/openshell-server/src/grpc/workspace.rs:223

DeleteWorkspace scans for blockers and deletes the workspace in separate operations. A concurrent resource creation can resolve the workspace and commit after the blocker scan, leaving a resource whose workspace no longer exists. Normal APIs will then refuse to resolve that workspace, making the resource effectively orphaned.

Use a persisted deleting state checked by resource creation, transactional exclusion, or another mechanism that closes the check/delete race.

[P2] Paginate sandbox-owned service cleanup

Location: crates/openshell-server/src/compute/mod.rs:1440

The new service-endpoint cleanup lists only the first 1,000 endpoints in the workspace and then deletes the sandbox. A sandbox whose endpoints fall after that page is deleted while its endpoints remain. Those orphaned records then block workspace deletion and can be presented as belonging to a later same-named sandbox.

Query endpoints by their persisted sandbox label, paginate until exhaustion, or store them under a sandbox-owned scope that supports delete_by_scope.

[P2] Keep Python workspace synchronization test-clean

Locations:

• python/openshell/sandbox.py:813
• python/openshell/sandbox_test.py:1555

The new unconditional self._session._workspace access breaks test_high_level_creation_forwards_name_and_labels: the high-level client double returns the public session shape used by this test but has no private _workspace field. The focused SDK run now fails with 74 passing tests and this one AttributeError.

Only resynchronize the workspace on the SandboxRef attachment path that needs it, or expose workspace through a stable session property and update the test double. The current head should not regress the existing Python unit suite.

[P2] Scope TUI profile joins by workspace

Locations:

• crates/openshell-tui/src/lib.rs:1975
• crates/openshell-tui/src/app.rs:2603

The all-workspaces provider view loads profiles only from current_workspace and applies them to every provider by provider.type. Provider-detail rendering then rejoins the fetched provider to cached entries using provider name alone. Same profile IDs or provider names in separate workspaces can therefore display another workspace's credentials, endpoints, discovery rules, and policy metadata.

Resolve profiles by (profile_workspace, profile_id) and identify providers by ID or (workspace, name).

[P2] Give all-workspace pagination a total ordering

Locations:

• crates/openshell-server/src/persistence/postgres.rs:440
• crates/openshell-server/src/persistence/sqlite.rs:476

Cross-workspace queries order results only by created_at_ms, name. Now that identical names may exist in separate workspaces, records created in the same millisecond can tie. Offset pagination over a non-total order can duplicate or omit resources, including during internal paginated scans.

Add workspace and id as deterministic tie-breakers to cross-workspace query ordering.

[P2] Persist workspace labels in the selector index

Location: crates/openshell-server/src/grpc/workspace.rs:118

CreateWorkspace places request labels in ObjectMeta but passes None for the persistence label column. ListWorkspaces.label_selector queries that persistence column, so it cannot find workspaces created with labels.

Serialize the already-validated workspace labels into the persistence label column before calling put_if.

[P2] Apply the new workspace-routing limit to generated sandbox names

Locations:

• crates/openshell-server/src/grpc/validation.rs:138
• crates/openshell-server/src/grpc/sandbox.rs:174

This PR lowers the explicit sandbox-name limit from 253 to 19 characters so workspace--sandbox--service fits in one DNS label. Sandbox-name validation still runs before petname generates a name for an unnamed request, so the server can persist a generated name that violates the new workspace-routing contract. Subsequent service operations reject that sandbox name even though the server generated it.

Validate the generated name and regenerate or fall back until it satisfies the new routable-name constraint.

[P2] Require managed-resource labels in driver ID lookups

Locations:

• crates/openshell-driver-kubernetes/src/driver.rs:451
• crates/openshell-driver-kubernetes/src/driver.rs:663
• crates/openshell-driver-podman/src/driver.rs:601

Several get, existence, and deletion paths filter only by the OpenShell sandbox ID. They can select an unmanaged platform object carrying the same label, and deletion may act on that object.

Include the OpenShell managed-by label in every lookup, not only list/watch paths.

Lower-priority feedback

[P2] Enforce persistence workspace invariants

Location: crates/openshell-server/src/persistence/mod.rs:240

The new raw put_if interface accepts a workspace separately from an encoded typed payload without verifying that it matches ObjectMeta.workspace. A caller error can produce a record whose database identity and decoded resource identity disagree. Derive or verify workspace at the typed persistence boundary; do not duplicate the existing GitHub thread about requiring a non-empty workspace on raw writes.

[P3] Complete the global-profile CLI surface

provider profile export, import, update, lint, and delete expose --global, while provider list-profiles always lists the selected workspace. Add an explicit global listing option.

[P3] Qualify platform-profile dependency diagnostics

Global-profile validation and deletion diagnostics identify dependent sandboxes only by name and then deduplicate those names. For example, dependencies on both alpha/work and beta/work are reported only as work. Return workspace/name so the operator can locate every blocker.

PR #2243 follow-up review

Principal-engineer verdict: request changes.

Reviewed the nine commits after the previous review, through head 0b1a19d5.

Prior findings

Critical findings

1. [P1] Workspace lifecycle remains TOCTOU/ABA-prone (CWE-362, CWE-367). Sandbox creation checks that the workspace is Active, performs several awaits, and only later inserts the sandbox. Deletion can mark, scan, and delete the workspace during that window, after which the creator inserts an orphan. The new test only starts creation after Terminating; it does not exercise an already in-flight creator. Concurrent deletions have a second ABA problem: their final membership cleanup and workspace delete use the name, so a delayed delete can remove a newly created same-name workspace. (active check, child insert, name-based deletion)

   Fix this at the persistence layer: child insertion and parent Active validation must share a transaction/row lock, while workspace deletion must validate and delete the original workspace UUID/version atomically. Add barrier-controlled race tests.

2. [P1] Migration 006 does not actually backfill stored resources. The SQL updates only the indexed workspace column, while decode_record explicitly leaves the protobuf payload's workspace empty. Migrated objects therefore query under default but return and update as workspace=""; typed CAS writes reject that empty workspace. This contradicts the PR's "existing resources backfilled to default" compatibility claim and is a concrete manifestation of prior finding Example: Run common tools from CLI tracker #11: raw put_if still accepts independent workspace and payload values. (migration, explicit breaking behavior, typed rejection)

   Re-encode legacy payloads during migration or hydrate and validate protobuf metadata from the authoritative database row. Test upgrading an actual v005 database through read, update, reconciliation, and deletion.

3. [P1] Existing Kubernetes sandboxes can be treated as missing and pruned from the gateway. Pre-PR managed CRs had the sandbox ID and managed-by labels but not the new sandbox-name/workspace fields. The new parser requires both; list silently drops failures and get converts them to None. Reconciliation can consequently delete the database record after the grace period while the Kubernetes workload remains running. (new parser/list behavior, required fields, pruning)

   Support legacy managed CRs using metadata.name and default, then patch the new fields, or provide an explicit upgrade migration.

4. [P1] An inference route makes a workspace permanently undeletable. Workspace deletion considers InferenceRoute a blocker, but the public service and CLI expose only Set/Get/Update—there is no delete or unset operation. The first deletion marks the workspace Terminating, after which Set is rejected, leaving no supported recovery path. (blocker, Set/Get-only API)

   Add Delete/UnsetInferenceRoute that remains usable during termination, or cascade-delete route configuration.

5. [P1] The 19-character limit still breaks first-party launchers. Gator's default is gator-YYYYMMDDHHMMSS, which is 20 characters and is passed directly to sandbox create. Several demos and published examples are longer still. The latest commit shortened tests but not these production paths. (Gator naming, prefix, validation)

Warnings

• [P2] PostgreSQL filtered all-workspace pagination is still nondeterministic. The unfiltered query gained workspace/id tie-breakers, but list_all_with_selector still orders only by timestamp and name. (query)

• [P2] Service cleanup still scans only the first 1,000 records. The follow-up adds a TODO, not a fix. A later endpoint survives and continues blocking workspace deletion. Adjacent: SSH-session cleanup has the identical first-page problem. (service cleanup, SSH cleanup)

• [P2] The corrected TUI profile join can freeze the UI. It performs one sequential five-second profile-list request per distinct workspace from inside the awaited tick handler—up to roughly 500 seconds for 100 providers—and truncates each workspace at 100 profiles. Use bounded concurrent exact-profile fetches or a batch API. (refresh loop, awaited tick)

• [P2] Python's default list semantics silently broadened. Existing client.list() callers now enumerate all workspaces instead of the default workspace. Use an explicit all_workspaces option and retain default as the compatibility default. The published label example also omits the newly required workspace argument. (SDK behavior, broken example)

Adjacent findings

• [Adjacent P2] SSH-session creation reads a Ready sandbox and inserts the session independently of sandbox cleanup, allowing an orphan session to be inserted after its parent is deleted. This is another CWE-362 parent/child race. (creation)

• [Adjacent P2] Kubernetes and Podman now locate a sandbox by immutable ID/labels but discard that identity and perform the destructive action by mutable name. A same-name replacement between lookup and delete can be affected instead (CWE-367). Retain Kubernetes UID/resourceVersion delete preconditions and use the Podman container ID. (Kubernetes, Podman)

Verification

• Python: 83 tests passed.
• Focused workspace server tests: 18 passed.
• Focused aggregator tests: 4 passed.
• TUI: 28 tests passed.
• None of these exercises the critical race, upgrade, legacy-CR, inference-route deletion, or multi-workspace TUI latency paths.
• The PR remains draft and conflicting; Branch Checks and Helm Lint were pending at the time of review.